-
Notifications
You must be signed in to change notification settings - Fork 21
use system libs by default #1077
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
use system libs by default #1077
Conversation
45090b7 to
0bd333b
Compare
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (89.47%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## develop #1077 +/- ##
===========================================
+ Coverage 86.19% 86.42% +0.23%
===========================================
Files 323 323
Lines 23954 23958 +4
===========================================
+ Hits 20646 20706 +60
+ Misses 3308 3252 -56 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
0bd333b to
53cefa2
Compare
8e86b01 to
e7ae03a
Compare
f739971 to
244d6ce
Compare
44a71c2 to
350cc17
Compare
|
| Scope | Lines Δ | Lines + | Lines - | Files Δ | Files + | Files ~ | Files ↔ | Files - |
|---|---|---|---|---|---|---|---|---|
| ⚙️ CI | 59 | 44 | 15 | 1 | - | 1 | - | - |
| 🛠️ Source | 48 | 30 | 18 | 6 | - | 6 | - | - |
| 🧪 Unit Tests | 46 | 41 | 5 | 4 | 1 | 3 | - | - |
| 🥇 Golden Tests | 43 | 21 | 22 | 6 | 1 | 5 | - | - |
| 📄 Docs | 31 | 25 | 6 | 3 | - | 3 | - | - |
| 🏗️ Build / Toolchain | 27 | 11 | 16 | 2 | - | 2 | - | - |
| 🧰 Tooling | 6 | 5 | 1 | 2 | - | 2 | - | - |
| Total | 260 | 177 | 83 | 24 | 2 | 22 | - | - |
Legend: Files + (added), Files ~ (modified), Files ↔ (renamed), Files - (removed)
🔝 Top Files
- .github/workflows/ci.yml (CI): 59 lines Δ (+44 / -15)
- test-files/golden-tests/symbols/function/sfinae.xml (Golden Tests): 34 lines Δ (+17 / -17)
- test-files/include/type_traits (Unit Tests): 32 lines Δ (+32 / -0)
bb8d10e to
be98305
Compare
a5c477b to
0c9fd10
Compare
3bf984d to
11a7b1e
Compare
|
An automated preview of the documentation is available at https://1077.mrdocs.prtest2.cppalliance.org/index.html If more commits are pushed to the pull request, the docs will rebuild at the same URL. 2026-01-13 12:47:49 UTC |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this PR has some content that is cognitively very hard to review it because it has many other changes unrelated to the main point of the PR, which is just changing the default. On the other hand, I understand that's the nature of the problem being solved here. 😅
I understand that changing the default is going to have implications in the sense that you have to do many other changes so that the new default doesn't break anything. And I also understand creating a PR for each of those changes would be huge and extremely unnecessary overhead. I don't want to impose any of that. I don't think we should impose rules where PRs cannot propose things in passing because that would create a necessary overhead for this project, and considering the actual state of the project, that's unreasonable. Maybe just separating commits that are unrelated to the problem is quite reasonable and makes review much easier (even though the GitHub UI unfortunately doesn't let us know where the change comes from if we choose to review the whole PR). 🙂
But from the point of reviewing it, because everything is in a single commit, it's really hard to review it because there are so many small changes in boring parts of the code far from each other involving build script, CI, and everything else. Those parts are disconnected, and there's no rationale for them. Anyone reviewing it just has to reverse engineer the code to figure out what's happening there. Even then, the person will figure out what's happening there, but not why that was necessary. Maybe another solution is just being more generous with comments, I guess. In fact I implemented a feature where we can now include comments in the C++ actions variables that for multi-line inputs like the case of the extended matrix handlebars templates. 😀
To be clear, I have no idea what the best solution for this is. But it's worth thinking about it. I mean all of us 😀. Maybe the best solution is just creating lots of commits and in the end, when the PR is about to be merged, we aggregate the commits that represent the same kind of feature into a single commit. So that the PR has rationale for the changes and the final PR has necessary structure for what users will see. But I'm not sure about any of that. We just have to think about that.
For now, for the small things, I'll just trust you to be honest. So, I'll just try to comment on whatever seems problematic to me because (maybe ironically) the bigger changes are easier to understand. At some point, I realized that was leaving a lot of comments just asking what things do and why they were there. I went back through my review at the end, deleting those comments because they were too repetitive. 😆
Anyway, the PR is pretty good. With the exception of the question about Mr.Docs' flags not being static for GCC, I think all other comments are mostly about ideas for brainstorming. (I also didn't really review the codecov issue because I know you're already on it)
cd8dd9f to
21ae97a
Compare
This change makes mrdocs use the system includes by default instead of the bundled ones. This helps mrdocs build projects which rely on differences between standard library implementations, since now mrdocs sees the source code closer to what the original compiler did, with only potential differences in the compilers themselves and some minor command line manipulations remaining. It also stops bundling the clang resource directory, relying on the one installed with libclang. This makes it so installing both to the same prefix be necessary, which is the same as all the other libclang based tools.
21ae97a to
c47c61c
Compare
8192afb to
c5e5469
Compare
alandefreitas
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
c5e5469 to
0b24ce6
Compare
0b24ce6 to
d14c59c
Compare
This change makes mrdocs use the system includes by default instead of the bundled ones.
This helps mrdocs build projects which rely on differences between standard library implementations, since now mrdocs sees the source code closer to what the original compiler did, with only potential differences in the compilers themselves and some minor command line manipulations remaining.
It also stops bundling the clang resource directory, relying on the one installed with libclang.
This makes it so installing both to the same prefix be necessary, which is the same as all the other libclang based tools.